Skip to content

fix(pve, v1.2): repair Python virtual environment in single-node deployment#6131

Open
xuang7 wants to merge 4 commits into
apache:release/v1.2from
xuang7:fix/pve-single-node-config-1.2
Open

fix(pve, v1.2): repair Python virtual environment in single-node deployment#6131
xuang7 wants to merge 4 commits into
apache:release/v1.2from
xuang7:fix/pve-single-node-config-1.2

Conversation

@xuang7

@xuang7 xuang7 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Python Virtual Environments (PVE) are broken in the single-node deployment on release/v1.2 for two independent reasons, fixed together here:

1. PVE REST calls returned 404 through single-node NGINX. The frontend requested /pve/... while the backend (PveResource, @Path("/pve"), Jersey mounted under /api/* on the coordinator) serves /api/pve/..., so every gateway had to carry its own /pve to /api/pve rewrite rule, and the single-node config was missing one. In the UI this surfaces as a "Could not load Python virtual environments" popup on every Python UDF operator selection.

Mirroring #6130 on main, this removes the mismatch at the source instead of adding a third rewrite rule: the frontend now builds PVE URLs with AppSettings.getApiEndpoint() and requests /api/pve/... directly, and every routing layer becomes a plain routing entry with no path rewriting:

  • bin/single-node/nginx.conf: location /api/pve/ passes through to the coordinator
  • frontend/proxy.config.json: /api/pve entry (ordered before /api), pathRewrite removed
  • bin/k8s/templates/gateway-routes.yaml: /api/pve joins the existing dynamic-backend match list, the URLRewrite filter is removed

This is an adapted port rather than a cherry-pick of the main commit: this branch predates the StoredPve db endpoints (/pve/db) and the gateway chart reorganization, so the same change lands in fewer files.

2. The system-package list was empty even with routing fixed. The system-requirements lock file is copied to /tmp/system-requirements-lock.txt in every deployed image (bin/computing-unit-master.dockerfile), but PveManager.getSystemPath chose its path from the isLocal flag (isLocal → amber/…, else /tmp/…). A containerized single-node run is also non-k8s, so isLocal is true and it looked for the repo-relative amber/system-requirements-lock.txt, which does not exist in the container → empty system-package set. The flag cannot distinguish a source-tree dev run from a containerized single-node run, so this probes for /tmp/system-requirements-lock.txt first and falls back to the repo-relative copy for dev runs.

Note: part 2 is release/v1.2-specific: main has a significantly refactored PveManager that does not use the lock file.

Any related issues, documentation, discussions?

Closes #5963. Ports #6130 (the /api/pve routing cleanup on main) to release/v1.2.

How was this PR tested?

On the single-node Docker stack (rc2 images + dashboard/coordinator rebuilt from this branch):

  • GET /api/pve/pves?cuid=1 returns 200 JSON from the coordinator; the old /pve path 404s to the dashboard catch-all as expected.
  • getSystemPath before/after: the unmodified rc2 coordinator returns {"system":[]}, this branch's coordinator returns the full system-package list.
  • The k8s route was applied to a minikube cluster: HTTPRoute Accepted, envoy routes /api/pve to the dynamic backend with no rewrite.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 4.8, Claude Fable 5)

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @SarahAsad23, @bobbai00
    You can notify them by mentioning @SarahAsad23, @bobbai00 in a comment.

@codecov-commenter

codecov-commenter commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.32%. Comparing base (a4dc934) to head (f496f71).
⚠️ Report is 12 commits behind head on release/v1.2.

Files with missing lines Patch % Lines
...virtual-environment/virtual-environment.service.ts 25.00% 3 Missing ⚠️
...resource/pythonvirtualenvironment/PveManager.scala 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                Coverage Diff                 @@
##             release/v1.2    #6131      +/-   ##
==================================================
- Coverage           53.60%   52.32%   -1.29%     
- Complexity           1440     2488    +1048     
==================================================
  Files                 797     1074     +277     
  Lines               33193    42116    +8923     
  Branches             3324     4535    +1211     
==================================================
+ Hits                17794    22036    +4242     
- Misses              14534    18775    +4241     
- Partials              865     1305     +440     
Flag Coverage Δ *Carryforward flag
access-control-service 64.61% <ø> (ø) Carriedforward from 447a235
agent-service 34.36% <ø> (ø) Carriedforward from 447a235
amber 52.48% <66.66%> (-7.03%) ⬇️
computing-unit-managing-service 1.65% <ø> (ø) Carriedforward from 447a235
config-service 56.06% <ø> (ø) Carriedforward from 447a235
file-service 58.59% <ø> (ø) Carriedforward from 447a235
frontend 47.13% <25.00%> (+0.81%) ⬆️
pyamber 90.72% <ø> (ø) Carriedforward from 447a235
python 90.75% <ø> (ø) Carriedforward from 447a235
workflow-compiling-service 58.69% <ø> (ø) Carriedforward from 447a235

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

xuang7 and others added 2 commits July 6, 2026 15:55
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
v1.2 adaptation of b864745 on main (PR apache#6130): the frontend builds PVE
URLs with AppSettings.getApiEndpoint(), so single-node nginx, the dev
proxy, and the k8s gateway all become plain routes with no rewriting.
Fewer files than on main because this branch predates the StoredPve db
endpoints and the gateway chart reorganization.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot added the frontend Changes related to the frontend GUI label Jul 7, 2026
@xuang7 xuang7 marked this pull request as ready for review July 10, 2026 17:47
@xuang7 xuang7 requested review from Yicong-Huang and kunwp1 July 10, 2026 17:47
Comment on lines +103 to +105
val deployPath = Paths.get("/tmp/system-requirements-lock.txt")
if (Files.exists(deployPath)) deployPath
else Paths.get("amber/system-requirements-lock.txt")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should rely on "system-requirements-lock.txt" any more. we removed it in #5608

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR targets the release branch, where the lock files are currently present.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. please annotate fix(pve, v1.2) in PR title so we know this is targeting v1.2

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Comment thread bin/single-node/nginx.conf Outdated
Comment on lines +98 to +103
location /api/pve/ {
proxy_pass http://workflow-runtime-coordinator-service:8085;
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be moved before /api

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, thanks.

@xuang7 xuang7 changed the title fix: repair Python virtual environment in single-node deployment fix(pve, v1.2): repair Python virtual environment in single-node deployment Jul 10, 2026
…tching

Mirrors the same change on the main PR (apache#6130) to keep the two branches
identical.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@xuang7 xuang7 force-pushed the fix/pve-single-node-config-1.2 branch from 99df8e4 to f496f71 Compare July 10, 2026 23:39
@xuang7 xuang7 requested a review from Yicong-Huang July 11, 2026 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine fix frontend Changes related to the frontend GUI infra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants